pull request: add required info #6231

0xc0170 · 2018-02-27T15:45:08Z

Description

Please use these 2 sections for describing a pull request. They should be part
of every pull request.

The type specifies what is expected from the pull request and when it can be
released. For instance a feature pull request can't be expected to go to the
patch release.

Important: do not mix pull request types !

I added also required to description (is Required placed there good, or any other suggestions?)

Pull request type

cmonr · 2018-02-27T15:47:19Z

.github/pull_request_template.md

 > Good example: https://os.mbed.com/docs/latest/reference/guidelines.html#workflow (Pull request template)

 # Pull request type

+> Required; Please tick one of the following types


Semicolons?

cmonr · 2018-02-27T15:49:02Z

.github/pull_request_template.md

@@ -1,10 +1,12 @@
 # Description

-> Detailed changes summary | testing | dependencies
+> Required; detailed changes summary | testing | dependencies


Is this suppose to read "detailed changes summary" or "detailed changes" "summary | testing | dependencies" ?

Any of these, just examples what to be expected. just that section is required

Oh, yes this ends up rendered weird as is. Thoughts on actual comments?

^ comment here ^

adbridge

Where does a breaking API change that is not a new feature fit here? Should we change 'feature' to
'Breaking change / New feature' ?

geky · 2018-02-27T15:50:36Z

.github/pull_request_template.md

 > Good example: https://os.mbed.com/docs/latest/reference/guidelines.html#workflow (Pull request template)

 # Pull request type

+> Required; Please tick one of the following types


Why not use actual comments?

https://stackoverflow.com/a/39737760

Good, let me test

@geky because we want to be able to automate the checking of the PR type and automatically assign the release tag

geky · 2018-02-27T15:52:05Z

@adbridge, IMO breaking change/new feature should be two separate things.

adbridge · 2018-02-27T15:52:54Z

@geky I would by happy with a separate entry for 'Breaking change'

Please use these 2 sections for describing a pull request. They should be part of every pull request. The type specifies what is expected from the pull request and when it can be released. For instance a feature pull request can't be expected to go to the patch release. Important: do not mix pull request types ! Changed also the heading type, to make it smaller.

adbridge

LGTM

cmonr · 2018-02-27T16:52:13Z

.github/pull_request_template.md

+
+<!-- 
+    Required
+    Please tick one of the following types 


With the additions of New target and Breaking change should this read "Please tick only one of the following" or "Please tick at least one of the following"?

@geky @adbridge ^^^

Only sounds fine. It should be one of these , not multiple selection here (if it is, PR should be split, doing multiple jobs).

I think the wording there is fine, please tick one , means just tick one :)

cmonr · 2018-02-27T19:21:53Z

@geky What was the rationale for having Breaking Change again? To me, any of the items in the list could be considered a breaking change.

Would you happen to have an example of what could be a breaking change, but not any of the other items? Or is the intention to have multiple checkmarks (ie: breaking change + fix)?

geky · 2018-02-27T19:52:39Z

Technically speaking, a "breaking change" is a break from documented behaviour. Although you could argue a significant change from "expected" behaivour is also a breaking change. Deciding what is a "breaking change" may require some thinking.

Increasing the size of mbed-os isn't necessarily a breaking change, since we don't document a maximum size, but suddenly increasing mbed-os's footprint to several hundred megabytes would be a breaking change, if only because then it couldn't fit on the platforms we support.

Features should not break documented behaviour. Changing directory iteration to skip every other file would be a "breaking change", but adding a new dir_read_skip_every_other_file function would just be a "feature addition".

It's mostly a matter of severity. The "breaking change" label should be a big red flag that the pr shouldn't be merged without a really good reason.

In theory "breaking changes" only get merged on major releases (not minor ones). However, our current rate of growth + distance between major releases has required some breaking changes in the past to be able to keep moving forward. As the code base becomes more mature breaking changes should become unacceptable outside of major releases.

Side note, a breaking change on a feature before it's made it to a minor release is a bit different, since it hasn't really been released yet. Not sure if those should also be tagged "breaking change" or just "feature" in that case.

adbridge · 2018-02-28T10:15:04Z

Breaking change is simple - it is an API change, not related to a new feature...

adbridge · 2018-02-28T15:32:41Z

/morph build

mbed-ci · 2018-02-28T16:07:11Z

Build : SUCCESS

Build number : 1298
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6231/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-02-28T17:56:02Z

Exporter Build : FAILURE

Build number : 959
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6231/

cmonr · 2018-02-28T17:58:13Z

Gotta love those ARM license server issues...

/morph export-build

mbed-ci · 2018-02-28T19:15:55Z

Exporter Build : SUCCESS

Build number : 961
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6231/

mbed-ci · 2018-02-28T19:54:41Z

Test : SUCCESS

Build number : 1084
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6231/1084

0xc0170 requested review from cmonr, sg-, geky, adbridge and AnotherButler February 27, 2018 15:45

0xc0170 added the needs: review label Feb 27, 2018

cmonr reviewed Feb 27, 2018

View reviewed changes

adbridge requested changes Feb 27, 2018

View reviewed changes

geky reviewed Feb 27, 2018

View reviewed changes

0xc0170 force-pushed the fix_pr_template_required branch 2 times, most recently from ecdc927 to dcfa6c3 Compare February 27, 2018 15:57

geky previously approved these changes Feb 27, 2018

View reviewed changes

0xc0170 dismissed geky’s stale review via c529b29 February 27, 2018 16:01

0xc0170 force-pushed the fix_pr_template_required branch from dcfa6c3 to c529b29 Compare February 27, 2018 16:01

0xc0170 force-pushed the fix_pr_template_required branch from c529b29 to 668f4d3 Compare February 27, 2018 16:06

geky approved these changes Feb 27, 2018

View reviewed changes

adbridge approved these changes Feb 27, 2018

View reviewed changes

sg- approved these changes Feb 27, 2018

View reviewed changes

cmonr reviewed Feb 27, 2018

View reviewed changes

adbridge added needs: CI and removed needs: review labels Feb 28, 2018

cmonr added ready for merge and removed needs: CI labels Feb 28, 2018

AnotherButler approved these changes Mar 1, 2018

View reviewed changes

cmonr added the release-version: 5.7.7 label Mar 1, 2018

adbridge merged commit 2b0f169 into ARMmbed:master Mar 1, 2018

adbridge removed the ready for merge label Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pull request: add required info #6231

pull request: add required info #6231

0xc0170 commented Feb 27, 2018 •

edited

cmonr Feb 27, 2018

cmonr Feb 27, 2018

0xc0170 Feb 27, 2018

geky Feb 27, 2018

adbridge left a comment

geky Feb 27, 2018

0xc0170 Feb 27, 2018

adbridge Feb 27, 2018

geky commented Feb 27, 2018

adbridge commented Feb 27, 2018

adbridge left a comment

cmonr Feb 27, 2018 •

edited

cmonr Feb 27, 2018

0xc0170 Feb 27, 2018

adbridge Feb 28, 2018

cmonr commented Feb 27, 2018 •

edited

geky commented Feb 27, 2018

adbridge commented Feb 28, 2018

adbridge commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

cmonr commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

pull request: add required info #6231

pull request: add required info #6231

Conversation

0xc0170 commented Feb 27, 2018 • edited

Description

Pull request type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adbridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Feb 27, 2018

adbridge commented Feb 27, 2018

adbridge left a comment

Choose a reason for hiding this comment

cmonr Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmonr commented Feb 27, 2018 • edited

geky commented Feb 27, 2018

adbridge commented Feb 28, 2018

adbridge commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

Triggering tests

mbed-ci commented Feb 28, 2018

cmonr commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

mbed-ci commented Feb 28, 2018

0xc0170 commented Feb 27, 2018 •

edited

cmonr Feb 27, 2018 •

edited

cmonr commented Feb 27, 2018 •

edited